-
Notifications
You must be signed in to change notification settings - Fork 373
feat(AnimationsProvider): Add Support for AnimationsProvider #11990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(AnimationsProvider): Add Support for AnimationsProvider #11990
Conversation
|
Preview: https://pf-react-pr-11990.surge.sh A11y report: https://pf-react-pr-11990-a11y.surge.sh |
…ons, add AnimationsProvider to helpers, add md files for documentation fix(components): update hasAnimations to fall back to context fix(md): remove cssPrefix fix(alertgroup): fix tests
855d346 to
49bcb04
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this! I have a handful of comments/change requests but they're mostly fairly small things.
This review didn't include looking at changing AlertGroup and DualListSelector from class based to function components that was needed as unfortunately I ran out of time. I wouldn't say to redo anything, but just for the sake of the future if a similar situation arises I would advocate for splitting that work into a distinct PR just to make review/testing easier, but that's just my $0.02.
packages/react-core/src/components/Alert/__tests__/AlertGroup.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Alert/__tests__/AlertGroup.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/DualListSelector/DualListSelector.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/helpers/AnimationsProvider/AnimationsProvider.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/helpers/AnimationsProvider/AnimationsProvider.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/helpers/AnimationsProvider/__tests__/AnimationsProvider.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/helpers/AnimationsProvider/__tests__/AnimationsProvider.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/helpers/AnimationsProvider/examples/AnimationsProvider.md
Outdated
Show resolved
Hide resolved
packages/react-core/src/helpers/AnimationsProvider/examples/AnimationsProviderBasic.tsx
Outdated
Show resolved
Hide resolved
| }); | ||
|
|
||
| // Animation context tests | ||
| test('respects AnimationsProvider context when no local hasAnimations prop', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of the opposite of Austin's comment about regarding a describe block, I personally might wrap these tests in a describe block specifically for the context tests. Not a blocker and depending how quickly we want to get this merged in, would like to hear @wise-king-sullyman opinion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so too @thatblindgeye - based on the RTL guide this seems like a good use case for a grouped describe() block, I've updated the tests to include it, but will also defer to @wise-king-sullyman here for the final word on whether or not we'd like to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against grouping associated things like the animation tests together in a describe block in an otherwise broader test suite, just wrapping full test suites in a describe block 🙂
| target.removeChild(this.state.container); | ||
| } | ||
| } | ||
| appendTo, // do not pass down to ul |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: it's pre-existing but I think we could remove this comment now that it's a functional component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, removed
packages/react-core/src/components/DualListSelector/DualListSelector.tsx
Outdated
Show resolved
Hide resolved
| const [focusAfterExpandChange, setFocusAfterExpandChange] = useState(false); | ||
|
|
||
| const { isExpanded, onToggleExpand, toggleAriaLabel, hasAnimations } = expandableInput || {}; | ||
| const { isExpanded, onToggleExpand, toggleAriaLabel, hasAnimations: localHasAnimations } = expandableInput || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a followup to my above reply to Austin about the prop name, I think this makes sense to call it localHasAnimations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just following up here to confirm based on the discussion in the thread here, I renamed it to hasAnimations for both the destructed and internal variable for consistency with our existing conventions for destructured props, for example JumpLinks uses activeIndex: activeIndexProp and isExpanded: isExpandedProp. What are your thoughts on using hasAnimationsProp here?
…e prop to hasAnimationsProp to match existing conventions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #11962
This PR enables support for
AnimationsProvider.PatternFly consumers will now be able to set animations for opt-in components once at the app level (instead of on every component via
hasAnimations) while maintaining full backward compatibility.Testing Instructions
To test the new context provider functionality:
Run
yarn startand navigate to http://localhost:8002/components/alert#toast-alert-groupCopy and paste the code below into the demo TSX code:
Click to expand
hasAnimationsprop.Additional Changes & Fixes
Converted class components to functional components to enable React Hooks usage
useHasAnimationshook integration:AlertGroup.tsx- Required foruseHasAnimationshook integrationDualListSelector.tsx- Required foruseHasAnimationshook integrationQuick Verification
hasAnimationsprop.<AnimationsProvider config={{ hasAnimations: true }}>hasAnimationsprop from the child component.Testing Note: Since PatternFly example files don't typically import from
/helpers, the ESLint configs require aReactimport with the// eslint-disable-next-line no-restricted-importscomment at the top of the file for local testing. Real consumers won't have this issue.Success Criteria
hasAnimationsprop overrides provider when set